Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(sdk) Add alias to file instead of overwriting #60

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Conversation

joewagner
Copy link
Contributor

This current aliases config helper will overwrite any existing aliases file when you create a new table. This was an oversight during initial development. This PR changes the behaviour to add to the file as new tables are created.

@linear
Copy link

linear bot commented Oct 24, 2023

JST-30 Add aliases to config file instead of overwriting

Summary

Currently, the SDK's aliases feature will overwrite the file specified with the jsonFileAliases helper function. This can be problematic because you risk losing past history of tables. It'd be better to append/push to the file rather than getting rid of what's already there.

Details

Update jsonFileAliases to add to the file instead of overwriting.

It also might but useful to have something like OpenZepplin's history implementation to add additional context to the alias tracking. Not required, just an idea—e.g., track the tx hash and the table name…just in case you lose track of what you did when more tables are appended.

{
   "alias": {
      "name": "alias_31337_2",
      "txnHash": "0x04406773e4c03db596be1f34efb5e96901571a2dde7c9025e389c438ab18b46c"
   },
   "test": {
      "name": "test_31337_3",
      "txnHash": "0x04406773e4c03db596be1f34efb5e96901571a2dde7c9025e389c438ab18b46c"
   }
}

// Or

{
  "0x04406773e4c03db596be1f34efb5e96901571a2dde7c9025e389c438ab18b46c": [
    "alias": "alias_31337_2",
    "test": "test_31337_3"
  ]
}

However, I suppose you could use SDK helpers to get that tx info from the table name/ID and troubleshoot from there, so maybe this is unneeded at this point.

@joewagner joewagner requested a review from dtbuchholz October 24, 2023 10:51
@@ -111,7 +111,8 @@ export function jsonFileAliases(filepath: string): AliasesNameMap {
},
write: async function (nameMap: NameMapping) {
const fs = await getFsModule();
fs.writeFileSync(filepath, JSON.stringify(nameMap));
const current = await this.read();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is really just this simple change. We should have done it this way from the start.
One note is that if you create two tables with the same name the new table will replace the old table, which seems correct to me.

@joewagner joewagner marked this pull request as ready for review October 24, 2023 10:52
Copy link
Contributor

@dtbuchholz dtbuchholz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@joewagner joewagner merged commit c13d818 into main Oct 24, 2023
4 checks passed
@joewagner joewagner deleted the joe/fix-jst-30 branch October 24, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants